Skip to content

ZOOKEEPER-3938: Upgrade JLine to 3.25.1 #2318

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
anmolnar merged 2 commits into from
Sep 30, 2025
Merged

ZOOKEEPER-3938: Upgrade JLine to 3.25.1 #2318

anmolnar merged 2 commits into from
Sep 30, 2025

Conversation

@PDavid
Copy link
Contributor

@PDavid PDavid commented Sep 24, 2025

JLine API has been changed between 2x and 3.x quite much so JLineZNodeCompleter and ZooKeeperMain had to be changed quite much.

@PDavid PDavid force-pushed the ZOOKEEPER-3938-jline-upgrade branch from f25b0f3 to 19da854 Compare September 24, 2025 14:40
@PDavid
Copy link
Contributor Author

PDavid commented Sep 24, 2025 •
edited
Loading

Tested this locally the following way:

Created config: cp conf/zoo_sample.cfg conf/zoo.cfg
Built the project: mvn clean install -DskipTests
Started ZooKeeper server: bin/zkServer.sh start
Connected to ZooKeeper server with the client: bin/zkCli.sh -server 127.0.0.1:2181
Created some test ZNodes

create /zk_test my_data
create /zk_test/whatever my_data2
create /zk_test/whatever/alma my_data3
create /zk_test/whatever/other my_data4

"JLine support is enabled" is displayed in the output - OK ✔️

Made sure that completion is working for commands:

2025-09-24 16:50:22,419 [myid:] - INFO  [main:o.a.z.ZooKeeper@1120] - Initiating client connection, connectString=127.0.0.1:2181 sessionTimeout=30000 watcher=org.apache.zookeeper.ZooKeeperMain$MyWatcher@7ea37dbf
2025-09-24 16:50:22,424 [myid:] - INFO  [main:o.a.z.c.X509Util@85] - Setting -D jdk.tls.rejectClientInitiatedRenegotiation=true to disable client-initiated TLS renegotiation
2025-09-24 16:50:22,529 [myid:] - INFO  [main:o.a.z.c.X509Util@109] - Default TLS protocol is TLSv1.3, supported TLS protocols are [TLSv1.3, TLSv1.2, TLSv1.1, TLSv1, SSLv3, SSLv2Hello]
2025-09-24 16:50:22,534 [myid:] - INFO  [main:o.a.z.ClientCnxnSocket@235] - jute.maxbuffer value is 1048575 Bytes
2025-09-24 16:50:22,538 [myid:] - INFO  [main:o.a.z.ClientCnxn@1674] - zookeeper.request.timeout value is 0. feature enabled=false
Welcome to ZooKeeper!
JLine support is enabled
2025-09-24 16:50:22,542 [myid:127.0.0.1:2181] - INFO  [main-SendThread(127.0.0.1:2181):o.a.z.ClientCnxn$SendThread@1121] - Opening socket connection to server localhost/127.0.0.1:2181.
2025-09-24 16:50:22,542 [myid:127.0.0.1:2181] - INFO  [main-SendThread(127.0.0.1:2181):o.a.z.ClientCnxn$SendThread@1123] - SASL config status: Will not attempt to authenticate using SASL (unknown error)
2025-09-24 16:50:22,548 [myid:127.0.0.1:2181] - INFO  [main-SendThread(127.0.0.1:2181):o.a.z.ClientCnxn$SendThread@974] - Socket connection established, initiating session, client: /127.0.0.1:55502, server: localhost/127.0.0.1:2181
2025-09-24 16:50:22,554 [myid:127.0.0.1:2181] - INFO  [main-SendThread(127.0.0.1:2181):o.a.z.ClientCnxn$SendThread@1378] - Session establishment complete on server localhost/127.0.0.1:2181, session id = 0x1000807a7230004, negotiated timeout = 30000

WATCHER::

WatchedEvent state:SyncConnected type:None path:null zxid: -1
[zk: 127.0.0.1:2181(CONNECTED) 0] add
addWatch   addauth

Also for ZNode path-s:

[zk: 127.0.0.1:2181(CONNECTED) 0] ls /z
/zk_test     /zookeeper

[zk: 127.0.0.1:2181(CONNECTED) 0] ls /zk_test/whatever/
/zk_test/whatever/alma    /zk_test/whatever/other

@PDavid
ZOOKEEPER-3938: Upgrade JLine to 3.25.1
965ca6c 
JLine API has been changed between 2x and 3.x quite much so JLineZNodeCompleter and ZooKeeperMain had to be changed quite much.
@PDavid PDavid force-pushed the ZOOKEEPER-3938-jline-upgrade branch from 19da854 to 965ca6c Compare September 24, 2025 15:15
anmolnar
anmolnar approved these changes Sep 24, 2025
View reviewed changes
Copy link
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff! lgtm.

@PDavid PDavid marked this pull request as ready for review September 24, 2025 17:21
@PDavid
Copy link
Contributor Author

PDavid commented Sep 24, 2025

org.apache.zookeeper.server.embedded.ZookeeperServerClusterTest.testStart failed in the PR build. I guess it is not related.

@ctubbsii
Copy link
Member

ctubbsii commented Sep 24, 2025

Are there any license file updates with this dependency update?

@PDavid
Copy link
Contributor Author

PDavid commented Sep 24, 2025

Are there any license file updates with this dependency update?

Thanks, good point. Updated the related licence files.

kezhuw
kezhuw reviewed Sep 30, 2025
View reviewed changes
pom.xml
<jetty.version>9.4.57.v20241219</jetty.version>
<jackson.version>2.15.2</jackson.version>
<jline.version>2.14.6</jline.version>
<jline.version>3.25.1</jline.version>
Copy link
Member

@kezhuw kezhuw Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not 3.30.6 ? The pr title is "ZOOKEEPER-3938: Upgrade JLine to 3.30.6" ?

Copy link
Contributor Author

@PDavid PDavid Sep 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, nice catch. 👍 Actually I had a problem with 30.30.6 that the completer was somehow not invoked. The 3.25.1 version worked fine and this version does not have the https://nvd.nist.gov/vuln/detail/CVE-2023-50572 anymore. But I forgot to update the PR title. :)

EDIT:
Updated the PR title now.

@PDavid PDavid changed the title ZOOKEEPER-3938: Upgrade JLine to 3.30.6 ZOOKEEPER-3938: Upgrade JLine to 3.25.1 Sep 30, 2025
@anmolnar anmolnar merged commit 9dbd958 into apache:master Sep 30, 2025
16 checks passed
@anmolnar
Copy link
Contributor

anmolnar commented Sep 30, 2025

Merged to master. Thanks @PDavid !
branch-3.9 has conflicts, please create separate pull request.

@PDavid
Copy link
Contributor Author

PDavid commented Oct 1, 2025

Merged to master. Thanks @PDavid ! branch-3.9 has conflicts, please create separate pull request.

Thanks @anmolnar. 👍 I'll create a PR for branch-3.9.

@PDavid PDavid deleted the ZOOKEEPER-3938-jline-upgrade branch October 1, 2025 09:25
PDavid added a commit to PDavid/zookeeper that referenced this pull request Oct 1, 2025
@PDavid
ZOOKEEPER-3938: Upgrade JLine to 3.25.1
33a1af6 
Reviewers: anmolnar, ctubbsii, kezhuw
Author: PDavid
Closes apache#2318 from PDavid/ZOOKEEPER-3938-jline-upgrade

(cherry picked from commit 9dbd958)
@PDavid PDavid mentioned this pull request Oct 1, 2025
Merged
@PDavid
Copy link
Contributor Author

PDavid commented Oct 1, 2025

The patch did not apply cleanly on branch-3.9 because of a slight local variable name difference in ZooKeeperMain.run(): "completer" vs. "completor" so I had to resolve the conflicts.

Prepared the PR for branch-3.9 here: #2322

@PDavid PDavid mentioned this pull request Oct 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kezhuw kezhuw kezhuw approved these changes

@ctubbsii ctubbsii ctubbsii approved these changes

@anmolnar anmolnar anmolnar approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@PDavid @ctubbsii @anmolnar @kezhuw